Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validator Proposer Rewrite #1462

Merged
merged 33 commits into from
Feb 5, 2019
Merged

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Feb 2, 2019

This PR implements the majority of the block proposal routine.

Key changes:

  • The proposal method has been moved to its own file as it is rather complex. The proposal method holds a lot of logic and the test file ends up being rather large to accommodate.
  • New RPC methods were stubbed to enable gathering certain data about the beacon chain, see Implement BeaconService.Eth1Data (server-side) #1463 and Implement BeaconServer.PendingDeposits (server-side) #1464.
  • There is an assumption on the attestation pool interface and there is no attestation pool provided at runtime. The validator will panic at runtime if selected to propose until the attestation pool implementation is provided.

@codecov
Copy link

codecov bot commented Feb 2, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@040405b). Click here to learn what that means.
The diff coverage is 79.16%.

@@            Coverage Diff            @@
##             master    #1462   +/-   ##
=========================================
  Coverage          ?   72.19%           
=========================================
  Files             ?      101           
  Lines             ?     6638           
  Branches          ?        0           
=========================================
  Hits              ?     4792           
  Misses            ?     1451           
  Partials          ?      395

@prestonvanloon prestonvanloon changed the title WIP - Validator Proposer rewrite Validator Proposer rewrite Feb 3, 2019
@rauljordan rauljordan changed the title Validator Proposer rewrite Validator Proposer Rewrite Feb 3, 2019
@prestonvanloon prestonvanloon requested a review from nisdas February 3, 2019 23:49
@@ -22,12 +23,14 @@ type ValidatorService struct {
conn *grpc.ClientConn
endpoint string
withCert string
p2p p2p.Broadcaster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validator client will touch p2p ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this design, yes. It's still under debate, I suppose, but this would be a trivial change if we wanted to offload broadcasting to the beacon chain node.

Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, but nice tests!

@@ -22,12 +23,14 @@ type ValidatorService struct {
conn *grpc.ClientConn
endpoint string
withCert string
p2p p2p.Broadcaster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rename this property? It clashes with the package name p2p and can be confusing to someone adding a method to this struct in the future. Also some IDEs complain.

@@ -0,0 +1,77 @@
package client

// Validator client proposer functions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a package level godoc comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, these are just the proposer related functions for the validator client. I will add a godoc package comment though

return
}

// Get pending deposit data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit more informative comment, perhaps: "fetch validator deposits into the ETH1.0 which have not yet been included in the beacon chain"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thinking, otherwise this comment is unnecessary since it doesn't add new information

return
}

// 2. Construct block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Punctuation in comments to maintain standard across repo

block.Signature = nil

// 5. Broadcast to the network
v.p2p.Broadcast(block)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in discord, do this in the chain service after block being received


// ProposeBlock
//
// WIP - not done.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer wip, more elaborate comment requested

@prestonvanloon prestonvanloon merged commit 4add403 into master Feb 5, 2019
@prestonvanloon prestonvanloon deleted the validator-proposer-rewrite branch February 5, 2019 13:47
@prestonvanloon prestonvanloon added this to the Sapphire milestone Feb 5, 2019
@rauljordan
Copy link
Contributor

#1510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants